Skip to content

tools: avoid rewriting same-version OpenSSL updates - #66999

Open
panva wants to merge 1 commit into
nodejs:mainfrom
panva:skip-existing-openssl-update
Open

tools: avoid rewriting same-version OpenSSL updates#66999
panva wants to merge 1 commit into
nodejs:mainfrom
panva:skip-existing-openssl-update

Conversation

@panva

@panva panva commented Oct 25, 2026 •
edited
Loading

Copy link
Copy Markdown
Member

Replay the existing update branch onto the current target branch before running the updater on scheduled runs. Its version check then skips the same version without rewriting the PR, preserving approvals and CI.

This also carries non-bot commits into a newer update. Manual dispatches bypass the replay so the update can still be regenerated.

@nodejs-github-bot

nodejs-github-bot commented Oct 25, 2026

Copy link
Copy Markdown
Collaborator

Review requested:

  • @nodejs/actions
  • @nodejs/security-wg

@nodejs-github-bot nodejs-github-bot added the meta Issues and PRs related to the general management of the project. label Oct 25, 2026
@panva panva added dont-land-on-v22.x PRs that should not land on the v22.x-staging branch and should not be released in v22.x. dont-land-on-v24.x PRs that should not land on the v24.x-staging branch and should not be released in v24.x. dont-land-on-v26.x PRs that should not land on the v26.x-staging branch and should not be released in v26.x. meta Issues and PRs related to the general management of the project. tools Issues and PRs related to the tools directory. and removed meta Issues and PRs related to the general management of the project. labels Oct 25, 2026
@panva panva changed the title tools: skip existing OpenSSL updates on schedule tools: avoid rewriting same-version OpenSSL updates Oct 25, 2026
@panva
panva force-pushed the skip-existing-openssl-update branch from e529655 to 3050fa8 Compare October 25, 2026 09:21
@aduh95

aduh95 commented Oct 25, 2026

Copy link
Copy Markdown
Contributor

I'd prefer we align it with #66503, for consistency. FWIW force pushing does not invalidate previous approvals nor CI, but yeah someone needs to force push back the previous commit for the CQ to detect it

@panva

panva commented Oct 25, 2026 •
edited
Loading

Copy link
Copy Markdown
Member Author

I believe non-bot commit presence being a skip condition is orthogonal to what this change is for. It can come too, but isn't resolving what I'm trying to resolve. And I think it's solved here too as a side effect of not pushing over the same version update.

aduh95
aduh95 reviewed Oct 25, 2026
View reviewed changes

@aduh95 aduh95 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think it's the right approach, we should fetch the PR_BRANCH first, apply it's changes before running the update-openssl.sh script

@panva
panva force-pushed the skip-existing-openssl-update branch from 3050fa8 to a81cea7 Compare October 25, 2026 16:58
@panva

panva commented Oct 25, 2026 •
edited
Loading

Copy link
Copy Markdown
Member Author

Updated to replay the existing update branch before running the script, so its version check handles the no-op. And it also keeps non-bot commits as they were. Manual dispatch bypasses the replay to allow regeneration.

@panva
panva requested a review from aduh95 October 29, 2026 09:35
aduh95
aduh95 reviewed Oct 29, 2026
View reviewed changes

@aduh95 aduh95 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't like the fetch-depth: 0, that's really wasteful. Also, this would pick up commits from the PR_BRANCH even if there's no open PR for it: in case the PR was closed but the ref wasn't deleted, we should ignore it.
I think we can solve both problems by making an API call:

Comment thread .github/workflows/update-openssl.yml Outdated
Comment thread .github/workflows/update-openssl.yml Outdated
@panva
panva force-pushed the skip-existing-openssl-update branch from a81cea7 to 729e83f Compare October 29, 2026 10:17
@panva

panva commented Oct 29, 2026

Copy link
Copy Markdown
Member Author

Updated to use the API. I used raw SHAs in an array because @sh would pass literal quotes to Git, and added --depth=2 to keep the fetch shallow. I left out || true because it would continue with an unfinished cherry-pick; we'd need to abort it first.

aduh95
aduh95 reviewed Oct 29, 2026
View reviewed changes
Comment thread .github/workflows/update-openssl.yml Outdated
Comment thread .github/workflows/update-openssl.yml Outdated
@panva
tools: avoid rewriting same-version OpenSSL updates
7f06241 
Replay the existing update branch onto the current target branch before
running the updater on scheduled runs. Its version check then skips the
same version without rewriting the PR, preserving approvals and CI.

This also carries contributor commits into a newer update. Manual
dispatches bypass the replay so the update can still be regenerated.

Signed-off-by: Filip Skokan <panva.ip@gmail.com>
Assisted-by: Codex
@panva
panva force-pushed the skip-existing-openssl-update branch from 729e83f to 7f06241 Compare October 29, 2026 11:06
@aduh95 aduh95 added author ready PRs with CI started, the required approvals, and no outstanding review comments. commit-queue PRs queued for automated landing through the Commit Queue. labels Oct 29, 2026
@aduh95

aduh95 commented Oct 29, 2026

Copy link
Copy Markdown
Contributor

Why the dont-land labels? Don't we want to backport this?

@nodejs-github-bot nodejs-github-bot added the lacks-second-approval Commit Queue PRs awaiting a second collaborator approval or completion of the required wait. label Oct 29, 2026
@panva

panva commented Oct 29, 2026 •
edited
Loading

Copy link
Copy Markdown
Member Author

Why the dont-land labels? Don't we want to backport this?

It'll need manual backport to 26.x like the previous fix to version sorting does. I have that in my backlog once 26.x-staging gets #66704, or ad-hoc if 3.5.10 releases before that (that will currently fail to get picked up).

@aduh95 aduh95 removed dont-land-on-v22.x PRs that should not land on the v22.x-staging branch and should not be released in v22.x. dont-land-on-v24.x PRs that should not land on the v24.x-staging branch and should not be released in v24.x. labels Oct 29, 2026
@aduh95 aduh95 added backport-blocked-v26.x PRs that should land on the v26.x-staging branch but are blocked by another PR's pending backport. and removed dont-land-on-v26.x PRs that should not land on the v26.x-staging branch and should not be released in v26.x. labels Oct 29, 2026
@aduh95

aduh95 commented Oct 29, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

Switched to backport-blocked instead

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@aduh95 aduh95 aduh95 approved these changes

Assignees

No one assigned

Labels

author ready PRs with CI started, the required approvals, and no outstanding review comments. backport-blocked-v26.x PRs that should land on the v26.x-staging branch but are blocked by another PR's pending backport. commit-queue PRs queued for automated landing through the Commit Queue. lacks-second-approval Commit Queue PRs awaiting a second collaborator approval or completion of the required wait. meta Issues and PRs related to the general management of the project. tools Issues and PRs related to the tools directory.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@panva @nodejs-github-bot @aduh95